Skip to content

Bring python 3.13 into integration tests#363

Merged
dkropachev merged 5 commits intoscylladb:masterfrom
dkropachev:int_tests_on_3.13
Dec 23, 2024
Merged

Bring python 3.13 into integration tests#363
dkropachev merged 5 commits intoscylladb:masterfrom
dkropachev:int_tests_on_3.13

Conversation

@dkropachev
Copy link
Collaborator

@dkropachev dkropachev commented Aug 9, 2024 •
edited
Loading

It is resurrection of #336
Brings python 3.13 to the integration tests.
Updates scylla to 5.4 and switches from gabrielfalcao/pyenv-action@v16 to actions/setup-python@v5

@dkropachev dkropachev marked this pull request as ready for review August 9, 2024 14:15
@dkropachev dkropachev requested review from Lorak-mmk, fruch and sylwiaszunejko and removed request for Lorak-mmk August 9, 2024 14:15
fruch
fruch approved these changes Aug 9, 2024
View reviewed changes
@dkropachev dkropachev self-assigned this Aug 9, 2024
@Lorak-mmk Lorak-mmk mentioned this pull request Aug 12, 2024
Closed
Lorak-mmk
Lorak-mmk requested changes Aug 12, 2024
View reviewed changes
Copy link

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: it is a good practice to have all commits passing tests. IIUC the first commit changes used Scylla version, which breaks some tests, and subsequent commits fix this breakage. If this is true, then could you move first commit to be the last? That would keep all commits passing.

@mykaul
Copy link

mykaul commented Aug 13, 2024

I've read somewhere that 3.13 optionally / experimentally removes the GIL. Would be good to test without it, to see where we are at.

@dkropachev dkropachev force-pushed the int_tests_on_3.13 branch 2 times, most recently from 2479b98 to 4e6329c Compare August 15, 2024 21:25
@dkropachev dkropachev requested a review from Lorak-mmk August 15, 2024 23:04
@dkropachev
Copy link
Collaborator Author

dkropachev commented Aug 16, 2024

I've read somewhere that 3.13 optionally / experimentally removes the GIL. Would be good to test without it, to see where we are at.

#370

@dkropachev
Copy link
Collaborator Author

dkropachev commented Aug 16, 2024 •
edited
Loading

Nit: it is a good practice to have all commits passing tests. IIUC the first commit changes used Scylla version, which breaks some tests, and subsequent commits fix this breakage. If this is true, then could you move first commit to be the last? That would keep all commits passing.

It is good idea, but, reality is that full test suit takes 30 minutes to complete, say you have 5 commits, it means that I have to spend 150 minutes on testing them.
Now, say I want to change something in the partilar commit, I have to go back to the commit I am changing, spend 30 minutes on testing it and, if code I have changed is not isolated, I have retest every commit that goes after it, does it worth it ? I think no

@Lorak-mmk
Copy link

Lorak-mmk commented Aug 20, 2024

Sure, it is not practical to run integration tests on each commit right now, because they take too long. But:

  • imo it is still worth it to try and have commits without braking tests - it makes it easier to read such commits and if there is a need to bisect something, there will be less fixing to do (even if there still is some fixing)
  • running unit tests will catch a lot of problems and is much faster.

Lorak-mmk
Lorak-mmk requested changes Aug 20, 2024
View reviewed changes
Copy link

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for the future: if you resolved an issue pointed by the comment, please mark the comment as resolved.

Lorak-mmk
Lorak-mmk reviewed Aug 20, 2024
View reviewed changes
Copy link

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing regarding comment tests/integration: introduced @xfail_scylla_version : commit message mentions xfail_scylla_version instead of xfail_scylla_version_lt

@fruch
Copy link

fruch commented Nov 6, 2024

@dkropachev

what was holding us from merging this one ?

@dkropachev
Copy link
Collaborator Author

dkropachev commented Nov 10, 2024

xfail_scylla_version_lt

Fixed.

Lorak-mmk
Lorak-mmk requested changes Nov 18, 2024
View reviewed changes
@dkropachev dkropachev force-pushed the int_tests_on_3.13 branch 2 times, most recently from 502250a to 5edb0e1 Compare November 18, 2024 21:14
@fruch
Copy link

fruch commented Dec 17, 2024

@dkropachev we need this merged, without this one, and the move to use scylla 5.4, we can't install scylla anymore.
cause of the java lookup is broken

@dkropachev dkropachev force-pushed the int_tests_on_3.13 branch 4 times, most recently from 2748e27 to 82f1bab Compare December 19, 2024 12:43
@dkropachev
Copy link
Collaborator Author

dkropachev commented Dec 20, 2024

@fruch , plan is to merge last pending PR: #397, rebase this PR and see what is left, which should be only pipeline changes.

@fruch @dkropachev
CI: add integration tests with python 3.13
ec2f58d 
since we want to start test the alpha/beta version of python
refactoring a bit the integration test workflow

the action for pyenv we are using isn't really getting updates
too much, and trying to switch back to the offical use python action
that can now have prerelease python versions
@dkropachev
rename assertNotEquals to assertNotEqual
cae1b82 
unittest renamed it and depricated assertNotEquals after 3.12
@dkropachev
Fix AsyncoreConnectionTests to check if asyncore present
0de2cd8 
Before 3.12 asyncore was present all the time.
Now we need to check if it is there.
@dkropachev
Fix test_ipv6 to test if asyncore is present
ba4e804 
asyncore is not present after 3.12
@dkropachev
Copy link
Collaborator Author

dkropachev commented Dec 22, 2024

@fruch , @Lorak-mmk after all rebases it is left only with python-related changes please take a look.

@dkropachev dkropachev requested a review from Lorak-mmk December 22, 2024 15:11
fruch
fruch approved these changes Dec 22, 2024
View reviewed changes
Copy link

@fruch fruch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dkropachev
Replace assertDictContainsSubset
58847ec 
assertDictContainsSubset was removed at python 3.12 we need to replace
it with alternative
@dkropachev dkropachev mentioned this pull request Dec 23, 2024
Merged
8 tasks
Lorak-mmk
Lorak-mmk approved these changes Dec 23, 2024
View reviewed changes
@dkropachev dkropachev merged commit a0ab1d3 into scylladb:master Dec 23, 2024
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fruch fruch fruch approved these changes

@Lorak-mmk Lorak-mmk Lorak-mmk approved these changes

@sylwiaszunejko sylwiaszunejko Awaiting requested review from sylwiaszunejko

Assignees

@dkropachev dkropachev

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dkropachev @mykaul @Lorak-mmk @fruch